Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refresh during destroy #27408

Merged
merged 4 commits into from
Jan 11, 2021
Merged

Refresh during destroy #27408

merged 4 commits into from
Jan 11, 2021

Conversation

jbardin
Copy link
Member

@jbardin jbardin commented Jan 5, 2021

Because the destroy plan only creates the necessary changes for apply to
remove all the resources, it does no reading of resources or data
sources, leading to stale data in the state. In most cases this is not a
problem, but when a provider configuration is using resource values, the
provider may not be able to run correctly during apply. In prior
versions of terraform, the implicit refresh that happened during
terraform destroy would update the data sources and remove missing
resources from state as required.

The destroy plan graph has a minimal amount of information, so it is not
feasible to work the reading of resources into the operation without
completely replicating the normal plan graph, and updating the plan
graph and all destroy node implementations is also a considerable amount
of refactoring. Instead, we can run a normal plan which is used to
refresh the state before creating the destroy plan. This brings back
similar behavior to core versions prior to 0.14, and the refresh can
still be skipped using the -refresh=false cli flag.

Fixes #27172

@jbardin jbardin requested a review from a team January 5, 2021 14:54
@codecov
Copy link

codecov bot commented Jan 5, 2021

Codecov Report

Merging #27408 (fb2208a) into master (d175e67) will increase coverage by 0.02%.
The diff coverage is 91.48%.

Impacted Files Coverage Δ
terraform/graph_builder_destroy_plan.go 91.42% <ø> (ø)
terraform/node_resource_abstract_instance.go 71.92% <60.00%> (-0.06%) ⬇️
terraform/context.go 90.51% <95.23%> (+0.20%) ⬆️
terraform/node_provider.go 87.50% <0.00%> (-1.14%) ⬇️
backend/local/testing.go 58.06% <0.00%> (+2.15%) ⬆️

Because the destroy plan only creates the necessary changes for apply to
remove all the resources, it does no reading of resources or data
sources, leading to stale data in the state. In most cases this is not a
problem, but when a provider configuration is using resource values, the
provider may not be able to run correctly during apply. In prior
versions of terraform, the implicit refresh that happened during
`terraform destroy` would update the data sources and remove missing
resources from state as required.

The destroy plan graph has a minimal amount of information, so it is not
feasible to work the reading of resources into the operation without
completely replicating the normal plan graph, and updating the plan
graph and all destroy node implementation is also a considerable amount
of refactoring. Instead, we can run a normal plan which is used to
refresh the state before creating the destroy plan. This brings back
similar behavior to core versions prior to 0.14, and the refresh can
still be skipped using the `-refresh=false` cli flag.
These tests were not previously running a refresh, and hence did not
expect the resources to be read.
get rid of the mutation of the plans.Plan and return values.
@jbardin jbardin force-pushed the jbardin/destroy-plan-refresh branch from 84ff97d to fb2208a Compare January 8, 2021 18:30
Copy link
Contributor

@mildwonkey mildwonkey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 makes sense to me! There are some really nice var name and comment updates in here, thank you!

@jbardin jbardin merged commit d26c149 into master Jan 11, 2021
@jbardin jbardin deleted the jbardin/destroy-plan-refresh branch January 11, 2021 19:24
@ghost
Copy link

ghost commented Feb 11, 2021

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@ghost ghost locked as resolved and limited conversation to collaborators Feb 11, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Data sources not refreshed on destroy
2 participants